Bugfix: swap="outerHTML" on <div> with style attribute leaves htmx-swapping class behind by matthews314 · Pull Request #3341 · bigskysoftware/htmx

matthews314 · 2025-06-18T16:59:01Z

Description

I stumbled upon an unexpected behavior a couple days ago while using HTMX version 1.9.10, but I found it happens in 1.9.12 and 2.0.4 as well.

Explanation by example

To explain it with an extremely stripped-down example, let's suppose you have a body like this:

<body>
	<a hx-get="/test" hx-target="#test-div" hx-swap="outerHTML">Click me</a>
	<div id="test-div" style></div>
</body>

Notice I'm trying to swap the div#test-div with an outerHTML swap strategy, and the div#test-div has a style attribute. All it takes is the attribute - there is no need for a string with actual style in it to be there.

Let's suppose the response from /test is this:

<div id="test-div">Test</div>

We should expect the resulting body to look like this:

<body>
	<a hx-get="/test" hx-target="#test-div" hx-swap="outerHTML">Click me</a>
	<div id="test-div">Test</div>
</body>

But actually, it looks like this:

<body>
	<a hx-get="/test" hx-target="#test-div" hx-swap="outerHTML">Click me</a>
	<div id="test-div" class="htmx-swapping">Test</div>
</body>

An htmx-swapping class is left behind.

The first regression test I added covers this case, and it fails if no changes are made to htmx.js (which is expected).

Root cause

I digged down in the source code using the debugger and found what seemed to be the root cause of the problem:

htmx/src/htmx.js

Line 1972 in d1d0cd9

target.classList.remove(htmx.config.swappingClass)

This line of code removes the htmx-swapping class from the target element, which is the original element in the DOM we're swapping. This works for innerHTML swaps because the node is still the same, but won't affect the new node in case of an outerHTML swap.

This is easily fixed by removing the htmx-swapping class from the new nodes too, which are in settleInfo.elts. This justifies the line 1992 I added. I'm pretty positive this is correct, but since I'm not an expert about the HTMX codebase, despite all tests are passing, I'm asking the maintainers to review this and confirm it won't have any unexpected side effects.

Why does the swap work when there's no `style` attribute on the original node, though?

Assume, from now on, I didn't implemented the line 1992 fix yet, because we need the code in the original state to keep debugging.

I removed style from the original node and scanned the code with the debugger to find where htmx-swapping was being removed. For the sake of clarity, I mean that we're now starting with this body:

<body>
	<a hx-get="/test" hx-target="#test-div" hx-swap="outerHTML">Click me</a>
	<div id="test-div"></div>
</body>

Notice no style attribute is on the div. Assume the endpoint response remains the same as before.

Turns out, the htmx-swapping class is removed during the second cloneAttributes call. This is the original cloneAttributes implementation:

// line 1409
/**
 * @param {Element} mergeTo
 * @param {Element} mergeFrom
 */
function cloneAttributes(mergeTo, mergeFrom) {
  forEach(mergeTo.attributes, function(attr) {
    if (!mergeFrom.hasAttribute(attr.name) && shouldSettleAttribute(attr.name)) {
      mergeTo.removeAttribute(attr.name)
    }
  })
  forEach(mergeFrom.attributes, function(attr) {
    if (shouldSettleAttribute(attr.name)) {
      mergeTo.setAttribute(attr.name, attr.value)
    }
  })
}
// line 1426

...

// line 732
/**
   * @template T
   * @param {T[]|NamedNodeMap|HTMLCollection|HTMLFormControlsCollection|ArrayLike<T>} arr
   * @param {(T) => void} func
   */
  function forEach(arr, func) {
    if (arr) {
      for (let i = 0; i < arr.length; i++) {
        func(arr[i])
      }
    }
  }
// line 745

I included the forEach implementation to make it easier to read.

cloneAttributes is called during the swap to make the new node (temporarily) look like the original one (meaning the style, class, width and height attributes of the original node are copied on the new node, or removed entirely if the original one doesn't have them).

Then it is called again (as a settleInfo.tasks element) during the settling phase, to reset the new node to its original appearance. To do this, it leverages newAttributes, a clone of the new node that has been created before modifying it. This last call is where htmx-swapping is wiped away when there is no style attribute.

This removal of htmx-swapping is due to the fact that the new node, originally, did not have any class on it. In practice, newAttributes node is exactly how /test responded:

<div id="test-div">Test</div>

So, when cloneAttributes is called on newNode and newAttributes, then newNode loses the class attribute, since there is no class attribute on newAttributes.

This has a side-effect: the htmx-added and the htmx-settling classes are removed as well. This looks like a bug to me, since there are a couple function calls further down the line that explicitly remove these two classes. I'll come back to this later.

How `cloneAttributes` behaves when there is the `style` attribute

I then added back the style attribute on the original node and debugged step by step the second cloneAttributes execution, to see why the wiping wasn't happening in this case.

What happens is that, being mergeTo.attributes and mergeFrom.attributes NamedNodeMaps, which (as per MDN docs) are live collections, when mergeTo.removeAttribute(attr.name) is called for style, the mergeTo.attributes array suddenly becomes one element shorter. On the next iteration inside the forEach function, the check on the length fails and the for loop is prematurely exited - the removal of the classes does not happen.

As I mentioned before, htmx-added and htmx-swapping are explicitly removed down the line, so this explains why only htmx-swapping is left behind on the new node.

I changed the cloneAttributes function to first create copies of mergeTo.attributes and mergeFrom.attributes (lines from 1415 to 1423 plus lines 1432 and 1437) so that the for loop is untouched by the changes that are being made to the NameNodeMaps. To be fair, only mergeTo.attributes required such a treatment, since mergeFrom is not being mutated. But I feel this way it is more future-proof and the performance impact should be negligible.

But now it's wiping all three `htmx-*` classes

It seems to me that cloneAttributes should be aware of the fact that htmx-* classes should be preserved: they're added by HTMX for a reason, and it's up to other parts of HTMX to get rid of them when it's done using them. So I added the lines from 1425 to 1430 and from 1443 to 1445.

I initially implemented something that preserved only htmx.config.addedClass, htmx.config.settlingClass and htmx.config.swappingClass (which are htmx-added, htmx-settling and htmx-swapping respectively). Also, this first version of mine only preserved the classes in case they were wiped out by the first forEach loop, but I soon realized the second forEach could wipe them as well. We just need to have a /test response that looks like this:

<div id="test-div" class="my-class">Test</div>

In this case, the second forEach would cause setAttribute('class', 'my-class') to be called, effectively obliterating all the htmx-* classes.

The code I ended up writing is both more concise and more conceptually sound, in my opinion. I'd like to know if maintainers think the same.

How to test the `cloneAttributes` fix?

As it stands before the fix, cloneAttributes does not remove the class attribute if it is preceded by a style (or width, or height) attribute.

So, even after the line 1992 fix, but before the cloneAttributes fix, we can still have the buggy behavior if the original node in the body looks like this:

<div id="test-div" style class="my-class"></div>

After the response from /test we would find the following div in the body:

<div id="test-div" class="my-class">Test</div>>

instead of the expected:

<div id="test-div">Test</div>

The regression test for the cloneAttributes fix covers exactly this scenario.

Corresponding issue

No corresponding issue to this PR. The guidelines said that, if I already have a solution, a PR is better suited, so here I am :)

Testing

I wrote the "swap=outerHTML clears htmx-swapping class when old node has a style attribute and no class" test first, which did not pass before the fix and passes after.

I realized the fix at line 1992 is all it takes to make this test pass, so we still missed a regression test for the fix in cloneAttributes

I added the "swap=outerHTML won't carry over user-defined classes when old node has a style attribute before the class attribute" test, which did not pass after the line 1992 fix alone, but passes after the cloneAttributes fix.

I manually checked (with the debugger) that htmx-added and htmx-settling now survive the second cloneAttributes call and are removed, respectively, here:

htmx/src/htmx.js

Line 1572 in d1d0cd9

removeClassFromElement(child, htmx.config.addedClass)

and here:

htmx/src/htmx.js

Line 1995 in d1d0cd9

elt.classList.remove(htmx.config.settlingClass)

I checked this in both the possible scenarios:

when the first forEach is the cause, i.e. when the original node does not have a class attribute;
the second forEach is the cause, i.e. when the original node has a class attribute with a user-defined class.

Checklist

I have read the contribution guidelines
I have targeted this PR against the correct branch (master for website changes, dev for
source changes)
This is either a bugfix, a documentation update, or a new feature that has been explicitly
approved via an issue
I ran the test suite locally (npm run test) and verified that it succeeded

… it failes, implemented initial fix in htmx.js that makes (all) test(s) run and pass.

…sts were passing and I know why, though. This means I miss one more regression test for the bug in cloneAttributes.

…butes after I noted they could as well be removed by mergeTo.setAttribute in the second forEach loop.

…explicit.

matthews314 · 2025-06-18T17:18:56Z

I accidentally forked the master branch only, so I had to merge dev in my branch to fix the conflicts, sorry.

Fortunately, it was just the line 1992 fix that needed to be moved inside the doSettle function (the function doesn't exist in the master branch yet) + the regression tests were conflicting because there was one more test in dev than there currently are in master.

MichaelWest22 · 2025-06-21T12:48:44Z

Yeah I recently did a deep dive of the cloneAttributes function when looking into an issue #412 where class attributes can be updated by third party libraries during the default settle delay which htmx then reverts breaking things. But I couldn't find a good solution to that problem. But I did find some of the same things you found about cloneAttributes. It does indeed remove all htmx related classes during this phase and I found some of these were later checked for and removed if they were still there. However I came to the conclusion that the after settle clone attributes call is meant to set the class to its final state which includes removing all the classes so later explicit removals are normally not needed (unless you disable class attribute settling). I don't think their is really a requirement that classes are kept on the elements at this stage as this is during the final stage of the settle and they are no longer needed here.

But iterating on live collection is a really good catch as this can be a real pain. I think we can simplify your fix down to just:

  function cloneAttributes(mergeTo, mergeFrom) {
    forEach(Array.from(mergeTo.attributes), function(attr) {
      if (!mergeFrom.hasAttribute(attr.name) && shouldSettleAttribute(attr.name)) {
        mergeTo.removeAttribute(attr.name)
      }
    })
    forEach(mergeFrom.attributes, function(attr) {
      if (shouldSettleAttribute(attr.name)) {
        mergeTo.setAttribute(attr.name, attr.value)
      }
    })
  }

Array.from() converts them to a stable array for us to iterate on safely and this passes the tests fine for me. The mergeFrom.attributes I found is actually stable here so it does not actually need to be copied to an array like the mergeTo does above.

With this one line change applied I was now unable to reproduce the issue and tests all passed fine.

Also did some manual testing and tried with attributesToSettle config set to not process class attribute which could cause issues but this worked fine as well and had no issue with retained htmx classes. I was unable to now reproduce the issue where swapping class was still on the new outerHTML replaced target that you added the extra remove class for all settle elements so I don't think we need this extra fix line you added at 1992 now either.

Anyway if you can update this PR I think this will be a great fix thanks.

matthews314 · 2025-06-25T09:01:21Z

Sorry for responding so late, busy days. Actually, I had the chance to read your response on saturday but I wasn't able to sit down and do the thing until now.

I see what you're saying, I just applied the changes and rerun the tests myself just to be sure. Funny how it boils down to a one liner :)
Thank you for taking the time to review it!

Matteo Smaila added 9 commits June 18, 2025 15:04

Added regression test for swap=outerHTML unexpected behavior, checked…

07db00f

… it failes, implemented initial fix in htmx.js that makes (all) test(s) run and pass.

Renamed variable in my regression test to be more clear.

b96aa89

I noticed I wasn't using the copies of the attributes I introduced.Te…

be5eeab

…sts were passing and I know why, though. This means I miss one more regression test for the bug in cloneAttributes.

Added one more regression test for the fix in cloneAttributes.

0234bb5

Made preservation of htmx- prefixed classes more robust in cloneAttri…

9aee1b3

…butes after I noted they could as well be removed by mergeTo.setAttribute in the second forEach loop.

Started as a typo-fix, ended up renaming regression tests to be more …

cf4b5a4

…explicit.

Started as a typo-fix, ended up renaming regression tests to be more …

893e151

…explicit.

Removed space that I accidentally added before.

c51ddf5

Merge branch 'dev' into swap-outer-html-bug

dce5fa1

MichaelWest22 added the bug Something isn't working label Jun 24, 2025

Applied changes as requested by MichaelWest22.

35035ca

MichaelWest22 approved these changes Jun 25, 2025

View reviewed changes

MichaelWest22 added the ready for review Issues that are ready to be considered for merging label Jun 25, 2025

Merge branch 'dev' into swap-outer-html-bug

0beac09

1cg merged commit b0c87bf into bigskysoftware:dev Jul 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Bugfix: swap="outerHTML" on <div> with style attribute leaves htmx-swapping class behind#3341

Bugfix: swap="outerHTML" on <div> with style attribute leaves htmx-swapping class behind#3341
1cg merged 11 commits into
bigskysoftware:devfrom
matthews314:swap-outer-html-bug

matthews314 commented Jun 18, 2025 •

edited

Loading

Uh oh!

matthews314 commented Jun 18, 2025

Uh oh!

MichaelWest22 commented Jun 21, 2025

Uh oh!

matthews314 commented Jun 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

matthews314 commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Explanation by example

Root cause

Why does the swap work when there's no style attribute on the original node, though?

How cloneAttributes behaves when there is the style attribute

But now it's wiping all three htmx-* classes

How to test the cloneAttributes fix?

Corresponding issue

Testing

Checklist

Uh oh!

matthews314 commented Jun 18, 2025

Uh oh!

MichaelWest22 commented Jun 21, 2025

Uh oh!

matthews314 commented Jun 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

matthews314 commented Jun 18, 2025 •

edited

Loading

Why does the swap work when there's no `style` attribute on the original node, though?

How `cloneAttributes` behaves when there is the `style` attribute

But now it's wiping all three `htmx-*` classes

How to test the `cloneAttributes` fix?